chore: Add StaticStates wrapper #377

mannycarrera4 · 2019-12-16T20:34:08Z

Fixes: #375

This is separating the work in #271

This creates a wrapper for stories to allow for treating pseudo-selectors like :hover or :focus as static states that are controllable as classes rather than the default pseudo-selectors.

For example, this is now a valid story:

.add('States', () => {
  <StaticStates>
    <Button className="focus">Focus</Button>
  </StaticStates>
});

This code will render a button with focus styling always applied. This is useful for enumerating states for design or validating states in visual regression tests.

To enable this functionality, Components must use the Canvas-kit styled function instead of the one from Emotion. The canvas-kit styled function also seamlessly handles a fallback to default theme and RTL support out-of-the-box.

modules/button/react/stories/stories_iconButton.tsx

modules/common/react/lib/utils/changeToStaticClass.ts

cypress · 2019-12-16T23:24:16Z

Test summary

56 • 0 • 0 • 0

Run details


Project	canvas-kit
Status	Passed
Commit	`92f4c47`
Started	Dec 26, 2019 8:11 PM
Ended	Dec 26, 2019 8:11 PM
Duration	00:46 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Electron 61

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

utils/storybook/CanvasProviderDecorator.tsx

modules/_labs/core/react/lib/CanvasProvider.tsx

modules/common/react/lib/utils/changeToStaticClass.ts

utils/storybook/CanvasProviderDecorator.tsx

anicholls · 2019-12-18T18:21:31Z

Thoughts on loading Default stories first and States stories last by updating storySort as part of this PR?

mannycarrera4 · 2019-12-18T18:23:10Z

@anicholls that makes sense to me. In the icon button PR, i had the states as a last story after seeing all the defaults!

modules/common/react/lib/utils/changeToStaticStates.ts

modules/button/react/stories/stories_iconButton.tsx

modules/common/react/lib/utils/changeToStaticStates.ts

modules/_labs/core/react/README.md

modules/button/react/lib/Button.tsx

modules/button/react/stories/stories.tsx

modules/common/react/spec/changeToStaticStates.spec.ts

anicholls · 2019-12-19T00:21:58Z

.storybook/config.js

  const prefixFn = pipe(
    prefix('welcome-', '0'),
    prefix('getting-started', '0'),
    prefix('tokens-', '1'),
-    prefix('components-', '2'),
+    prefix('components-', '3'),
+    prefix('states', '4'),


This works, but I think we may want to do this separately from this calculation. This is specifically meant to target the prefix from the categories at the top level of storybook. I believe this could result in some unexpected edge cases for something like a story named Tokens|Foo/Bar/States. I would suggest doing a separate check after the number prefix has already been added.

This function replaced another function that only handled top-level categories. The input of that function was something like ['Welcome', 'Tokens', 'Components'], but I also needed to make sure "Getting Started" was the first item under "Welcome" so that Storybook picked it as the default page. Numbers were chosen because sorting alphabetically ensures numbers rank higher than letters, so '0first' will come before 'first'. Also numbers are easy for us to understand in terms of "ranking".

This function is definitely a hack, but the intent is to be powerful enough to handle sorting on a global level while still being easy enough to add new sorting rules. I actually think adding prefix('states', '3') before the prefix('components-', '3') would accomplish the same goal without introducing a new sorting hierarchy ('3' would be prefixed for having 'states' in the storyname first, then '3' would be prefixed first for being in the "Components" category).

Actually, I retract the 3. It has to be a 4 to not be the first story in Components... Perhaps not very intuitive

.storybook/config.js

modules/common/react/lib/utils/convertPseudoStatesBehavior.ts

anicholls · 2019-12-19T19:56:54Z

I just realized that we have the same problem with this that we did when we tried to add a dir prop to components for RTL. It worked in the example case you tried (buttons) because we only have states on the button element and we spread extra props on that element. But for components with pseudo states in multiple nested styled components, we need to ensure the _shouldConvertPseudoStatesToClasses prop is passed to every styled component.

@NicholasBoll I think this is probably okay to go in as is, but we really need to look at re-writing the styled factory so we can inject our own props like this. If we don't, it's going to add a lot of noise/complexity to our components very quickly.

NicholasBoll · 2019-12-19T22:12:54Z

I just realized that we have the same problem with this that we did when we tried to add a dir prop to components for RTL. It worked in the example case you tried (buttons) because we only have states on the button element and we spread extra props on that element. But for components with pseudo states in multiple nested styled components, we need to ensure the _shouldConvertPseudoStatesToClasses prop is provided for every styled component.

@NicholasBoll I think this is probably okay to go in as is, but we really need to look at re-writing the styled factory so we can inject our own props like this. If we don't, it's going to add a lot of noise/complexity to our components very quickly.

Agreed. I think the ideal implementation is using a Context where a Provider (we originally called it StaticState) when wrapping a component will provide a context value that initiates the static state functionality and put it in the StyledComponent component. This way, the functionality is easy to initiate from the documentation in a way that each component doesn't have to know anything about.

We tried it in the styled.ts factory function, but Context is only available to components. We'd either have to create a new wrapper for every component or rewrite the styled function from emotion to add this functionality to the StyledComponent component. I didn't like the idea of creating an extra component wrapper around every styled component and it also sounds like more maintenance for our own version of styled.

For a better developer experience, I think we'd take on the maintenance burden of maintaining our own styled wrapper to make it seamless.

NicholasBoll · 2019-12-20T18:51:38Z

After talking with @anicholls, it is possible to use our styled from the labs/themes as long as we use the theme context which is provided to our interpolations. We are already doing this with RTL, so we're just tacking on.

I opted out of having the boolean parameter part of CanvasTheme, so it is a private boolean.

NicholasBoll · 2019-12-20T18:55:11Z

.storybook/config.js

-const prefix = (phrase, prefix) => value => (value.indexOf(phrase) > -1 ? prefix + value : value);
+const prefix = (phrase, prefix) => (/** @type {string} */ value) => {
+  const index = value.indexOf(phrase);
+  return index > -1 ? value.substr(0, index) + prefix + value.substr(index) : value;


This change allows for multi-tier sorting instead of a global sorting. For example, "Welcome" is the category and "getting-started" is the item. The below will produce a string like:

"0welcome--agetting-started"

Since the "a" is now added before "getting-started" instead of before "welcome", it allows sorting to be local to other items. Lowercase letters should be used in multi-tier sorting so it flows naturally with other items without disrupting them. For example, numbers are always before lowercase letters in ASCII. So even if you add a '9', it will sort first rather than the desired last which is why z is used below

NicholasBoll · 2019-12-20T18:56:46Z

modules/_labs/core/react/lib/theming/styled.ts

+      .replace(/^:/, '&:') // handle shorthand like ":focus"
+      .replace(/,(\s+):/g, ',$1&:') // handle selectors like ":focus, :hover"
+      .replace(/:(focus|hover|active)/g, '.$1')
+      .replace(/\[data\-whatinput="?(mouse|touch|keyboard|pointer)"?]/g, '[data-whatinput="noop"]');


Disables the InputProvider style overrides

NicholasBoll · 2019-12-20T18:57:10Z

modules/_labs/core/react/lib/theming/styled.ts

 function styled<Props>(node: any) {
  return (...args: Interpolation<Props>[]) => {
    const newArgs: Interpolations = args.map(
-      interpolation => (props: Props & {theme: CanvasTheme; direction: ContentDirection}) => {
+      interpolation => (props: Props & {theme: CanvasTheme & {_staticStates?: boolean}}) => {


Not adding _staticStates to CanvasTheme keeps it private

Changes requested are now outdated

mannycarrera4 commented Dec 16, 2019

View reviewed changes

modules/button/react/stories/stories_iconButton.tsx Outdated Show resolved Hide resolved

mannycarrera4 commented Dec 16, 2019

View reviewed changes

modules/common/react/lib/utils/changeToStaticClass.ts Outdated Show resolved Hide resolved

NicholasBoll approved these changes Dec 16, 2019

View reviewed changes

mannycarrera4 added 8 commits December 16, 2019 16:07

chore: Refactor canvasProvider and extract util function

cb165ca

chore: Rename function

acad35d

fix: Rename prop name

47e16e0

fix: Remove unused import

0ae64b3

fix: Remove space added

e2eb14d

chore: Rebase and rename prop

76bac48

fix: Fix merging conflicts

f48e7ce

fix: Rename prop for decorator

d49dc80

anicholls previously requested changes Dec 16, 2019

View reviewed changes

fix: Rename prop and add readme

39a06f1

mannycarrera4 commented Dec 17, 2019

View reviewed changes

utils/storybook/CanvasProviderDecorator.tsx Outdated Show resolved Hide resolved

mannycarrera4 mentioned this pull request Dec 17, 2019

fix(button): Address issues with various IconButton states #271

Merged

NicholasBoll and others added 3 commits December 17, 2019 15:55

feat: Add wrapper for staticState utility function

b57196e

fix: Add tests for changeToStaticStates and refactor decorator

cf4e352

fix: Remove storybook parameter for testing

45b512b

lychyi added this to In progress in Current Sprint (7/20 - 8/9) via automation Dec 18, 2019

mannycarrera4 added 2 commits December 18, 2019 13:54

fix: Remove test code for buttons

7e311d8

fix: Sort state stories last

c845fde

anicholls reviewed Dec 18, 2019

View reviewed changes

anicholls reviewed Dec 19, 2019

View reviewed changes

mannycarrera4 added 4 commits December 19, 2019 09:33

fix: Rename function for converting pseudo to class

5c8e2db

fix: Remove console.log and add back in stories

cd0d94e

fix: Change styles to interpolation

7d5c375

test: Add test for &:state

10285a0

anicholls reviewed Dec 19, 2019

View reviewed changes

modules/common/react/lib/utils/convertPseudoStatesBehavior.ts Outdated Show resolved Hide resolved

fix: Change name of interface

63821d7

mannycarrera4 and others added 4 commits December 19, 2019 15:38

fix: Nicholas is dope

d904496

Merge branch 'master' into mc-update-canvas-provider

4e74004

chore: Removed unneeded type from styled.ts

4b2952b

feat: Move static state conversion into our theme

d71362a

NicholasBoll changed the title ~~chore: Update CanvasProvider and add util for static pseudo states~~ chore: Update CanvasProvider and add StaticStates wrapper Dec 20, 2019

NicholasBoll reviewed Dec 20, 2019

View reviewed changes

fix: Remove disable input provider prop

413e5b6

mannycarrera4 changed the title ~~chore: Update CanvasProvider and add StaticStates wrapper~~ chore: Add StaticStates wrapper Dec 24, 2019

Merge branch 'master' into mc-update-canvas-provider

eb73ce4

NicholasBoll merged commit ed41a76 into Workday:master Dec 26, 2019

Current Sprint (7/20 - 8/9) automation moved this from In progress to Done Dec 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Add StaticStates wrapper #377

chore: Add StaticStates wrapper #377

mannycarrera4 commented Dec 16, 2019 •

edited by NicholasBoll

cypress bot commented Dec 16, 2019 •

edited

anicholls commented Dec 18, 2019

mannycarrera4 commented Dec 18, 2019

anicholls Dec 19, 2019

NicholasBoll Dec 19, 2019

NicholasBoll Dec 19, 2019

anicholls commented Dec 19, 2019 •

edited

NicholasBoll commented Dec 19, 2019

NicholasBoll commented Dec 20, 2019

NicholasBoll Dec 20, 2019

NicholasBoll Dec 20, 2019

NicholasBoll Dec 20, 2019

chore: Add StaticStates wrapper #377

chore: Add StaticStates wrapper #377

Conversation

mannycarrera4 commented Dec 16, 2019 • edited by NicholasBoll

cypress bot commented Dec 16, 2019 • edited

Test summary

Run details

anicholls commented Dec 18, 2019

mannycarrera4 commented Dec 18, 2019

anicholls Dec 19, 2019

Choose a reason for hiding this comment

NicholasBoll Dec 19, 2019

Choose a reason for hiding this comment

NicholasBoll Dec 19, 2019

Choose a reason for hiding this comment

anicholls commented Dec 19, 2019 • edited

NicholasBoll commented Dec 19, 2019

NicholasBoll commented Dec 20, 2019

NicholasBoll Dec 20, 2019

Choose a reason for hiding this comment

NicholasBoll Dec 20, 2019

Choose a reason for hiding this comment

NicholasBoll Dec 20, 2019

Choose a reason for hiding this comment

mannycarrera4 commented Dec 16, 2019 •

edited by NicholasBoll

cypress bot commented Dec 16, 2019 •

edited

anicholls commented Dec 19, 2019 •

edited